Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test(client): wrap cmd.SetArgs to fix bugs for cmd.SetArgs #18876

Merged
merged 5 commits into from
Dec 26, 2023
Merged

test(client): wrap cmd.SetArgs to fix bugs for cmd.SetArgs #18876

merged 5 commits into from
Dec 26, 2023

Conversation

levisyin
Copy link
Contributor

@levisyin levisyin commented Dec 23, 2023 •
edited
Loading

Description

This pr is aim to provide a simple way testutil.SetArgs to replace cmd.SetArgs in test as there is bug for cmd.SetArgs, see spf13/cobra/issues/2079 for more detail info about the bug.

Reason

Refer to pr #18557 added test, it's fussy to explicitly set all args that has changed before in writing unit test. We should concentrate on the new test case, no need to care about what this case's args will take effects to other test cases.

Test

I have replaced cmd.SetArgs to testutil.SetArgs in keys show test, it works well. If this pr LGTY, I will draft a new pr to check if there are test cases that depend on the bug behavior of SetArgs, and replace cmd.SetArgs to testutil.SetArgs for all test file.

@levisyin levisyin requested a review from a team as a code owner December 23, 2023 04:34
@coderabbitai coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 23, 2023 •
edited
Loading

Walkthrough

The codebase underwent a refactor where the SetArgs function was moved from a test file in the client/keys directory to a utility file in the testutil package. This move was intended to resolve issues with resetting flag values during testing. The runShowCmd function's tests were updated to use the new testutil.SetArgs method. Additionally, two new tests were added to demonstrate the differences between the original cobra.Command.SetArgs method and the new wrapped method.

Changes

Files Change Summary
client/keys/show_test.go
testutil/cmd.go
testutil/cmd_test.go		 Moved SetArgs to cmd.go and added new tests for argument setting methods.
Replaced cmd.SetArgs with testutil.SetArgs in test functions.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ?

Tips

Chat with CodeRabbit Bot (@coderabbitai)

Note: Auto-reply has been disabled for this repository by the repository owner. The CodeRabbit bot will not respond to your comments unless it is explicitly tagged.

  • You can tag CodeRabbit on specific lines of code or entire files in the PR by tagging @coderabbitai in a comment. Examples:
  • You can tag @coderabbitai in a PR comment and ask questions about the PR and the codebase. Use quoted replies to pass the context for follow-up questions. Examples:
    • @coderabbitai render interesting statistics about this repository as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai generate unit tests for the src/utils.ts file.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

@github-actions github-actions bot added C:CLI C:Keys Keybase, KMS and HSMs labels Dec 23, 2023
coderabbitai[bot]
coderabbitai bot reviewed Dec 23, 2023
View reviewed changes
testutil/cmd.go Outdated Show resolved Hide resolved
coderabbitai[bot]
coderabbitai bot reviewed Dec 23, 2023
View reviewed changes
testutil/cmd_test.go Outdated Show resolved Hide resolved
@levisyin @coderabbitai
Update testutil/cmd_test.go
d75ef08 
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
julienrbrt
julienrbrt reviewed Dec 23, 2023
View reviewed changes
testutil/cmd.go Outdated
// SetArgs sets arguments for the command. It is desired to replace the cmd.SetArgs in all test case, as cmd.SetArgs doesn't reset flag value as expected.
//
// see https://github.com/spf13/cobra/issues/2079#issuecomment-1867991505 for more detail info
func SetArgs(cmd *cobra.Command, args []string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be mindful of new public api we add. Can we maybe have this in an internal package?

Concerning using it everywhere in the sdk, we should really really watch out that we don't increase the reliance on the sdk for already extracted modules.

Copy link
Contributor Author

@levisyin levisyin Dec 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@julienrbrt Hi sir, I have moved it to internal/testutil

@julienrbrt julienrbrt changed the title refactor(tests): wrap cmd.SerArgs to fix bugs for cmd.SetArgs test(client) wrap cmd.SetArgs to fix bugs for cmd.SetArgs Dec 23, 2023
coderabbitai[bot]
coderabbitai bot reviewed Dec 23, 2023
View reviewed changes
internal/testutil/cmd.go Show resolved Hide resolved
internal/testutil/cmd_test.go Show resolved Hide resolved
internal/testutil/cmd_test.go Show resolved Hide resolved
@levisyin levisyin changed the title test(client) wrap cmd.SetArgs to fix bugs for cmd.SetArgs test(client): wrap cmd.SetArgs to fix bugs for cmd.SetArgs Dec 23, 2023
julienrbrt
julienrbrt approved these changes Dec 23, 2023
View reviewed changes
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, ty!

@tac0turtle tac0turtle added this pull request to the merge queue Dec 26, 2023
Merged via the queue into cosmos:main with commit ae19acc Dec 26, 2023
55 of 57 checks passed
@levisyin levisyin deleted the refactor/cmd-setargs branch December 27, 2023 06:57
@levisyin levisyin mentioned this pull request Dec 27, 2023
Closed
levisyin added a commit to levisyin/cosmos-sdk that referenced this pull request Dec 27, 2023
@levisyin
Revert "test(client): wrap cmd.SetArgs to fix bugs for cmd.SetArgs (c…
d119740 
…osmos#18876)"

This reverts commit ae19acc.
@levisyin levisyin mentioned this pull request Dec 27, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jan 3, 2024
@levisyin
revert: test(client) wrap cmd.SetArgs to fix bugs for cmd.SetArgs (#1…
77cbb3c 
…8876) (#18902)
@levisyin levisyin mentioned this pull request Jan 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@julienrbrt julienrbrt julienrbrt approved these changes

@tac0turtle tac0turtle tac0turtle approved these changes

Assignees
No one assigned
Labels
C:CLI C:Keys Keybase, KMS and HSMs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@levisyin @tac0turtle @julienrbrt